-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update dskit and add TLS cipher suites and minimum version support for clients #3070
Conversation
…r clients Signed-off-by: Marco Pracucci <marco@pracucci.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could we put grpc-client config into a separate documentation block to avoid repeating all the fields multiple times in rendered documentation?
The gRPC client config is already in a separate doc block. I wanted to put TLS in a separate doc, but we include it "inline" so can't be put in a separate block. Am I missing anything? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we put grpc-client config into a separate documentation block to avoid repeating all the fields multiple times in rendered documentation?
The gRPC client config is already in a separate doc block. I wanted to put TLS in a separate doc, but we include it "inline" so can't be put in a separate block.
Am I missing anything?
You are right. I got confused by seeing that other (non-grpc) client configs (-alertmanager.alertmanager-client
or -ruler.alertmanager-client
)
The TLS cipher suites supported by Mimir are: | ||
|
||
- `TLS_RSA_WITH_AES_128_CBC_SHA` | ||
- `TLS_RSA_WITH_AES_256_CBC_SHA` | ||
- `TLS_RSA_WITH_AES_128_GCM_SHA256` | ||
- `TLS_RSA_WITH_AES_256_GCM_SHA384` | ||
- `TLS_AES_128_GCM_SHA256` | ||
- `TLS_AES_256_GCM_SHA384` | ||
- `TLS_CHACHA20_POLY1305_SHA256` | ||
- `TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA` | ||
- `TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA` | ||
- `TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA` | ||
- `TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA` | ||
- `TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256` | ||
- `TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384` | ||
- `TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256` | ||
- `TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384` | ||
- `TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256` | ||
- `TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256` | ||
- `TLS_RSA_WITH_RC4_128_SHA` (insecure) | ||
- `TLS_RSA_WITH_3DES_EDE_CBC_SHA` (insecure) | ||
- `TLS_RSA_WITH_AES_128_CBC_SHA256` (insecure) | ||
- `TLS_ECDHE_ECDSA_WITH_RC4_128_SHA` (insecure) | ||
- `TLS_ECDHE_RSA_WITH_RC4_128_SHA` (insecure) | ||
- `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` (insecure) | ||
- `TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256` (insecure) | ||
- `TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256` (insecure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should include this list in the generic documentation. It can change between Go versions, and we may not update it properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a valid concern. What's about 2a51144 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
… because it could go out of sync soon Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
# (advanced) Override the default cipher suite list (separated by commas). | ||
# Allowed values: | ||
# | ||
# Secure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Secure Ciphers: | |
# Secure ciphers: |
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# | ||
# Insecure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Insecure Ciphers: | |
# Insecure ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are auto-generated from code in another library. I will change it there.
# (advanced) Override the default cipher suite list (separated by commas). | ||
# Allowed values: | ||
# | ||
# Secure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Secure Ciphers: | |
# Secure ciphers: |
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# | ||
# Insecure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Insecure Ciphers: | |
# Insecure ciphers: |
# (advanced) Override the default cipher suite list (separated by commas). | ||
# Allowed values: | ||
# | ||
# Secure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Secure Ciphers: | |
# Secure ciphers: |
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# | ||
# Insecure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Insecure Ciphers: | |
# Insecure ciphers: |
# (advanced) Override the default cipher suite list (separated by commas). | ||
# Allowed values: | ||
# | ||
# Secure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Secure Ciphers: | |
# Secure ciphers: |
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# | ||
# Insecure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Insecure Ciphers: | |
# Insecure ciphers: |
# (advanced) Override the default cipher suite list (separated by commas). | ||
# Allowed values: | ||
# | ||
# Secure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Secure Ciphers: | |
# Secure ciphers: |
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# | ||
# Insecure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Insecure Ciphers: | |
# Insecure ciphers: |
# (advanced) Override the default cipher suite list (separated by commas). | ||
# Allowed values: | ||
# | ||
# Secure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Secure Ciphers: | |
# Secure ciphers: |
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 | ||
# | ||
# Insecure Ciphers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Insecure Ciphers: | |
# Insecure ciphers: |
docs/sources/operators-guide/secure/securing-communications-with-tls.md
Outdated
Show resolved
Hide resolved
docs/sources/operators-guide/secure/securing-communications-with-tls.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with minor nits that I found along the way while looking at every line.
Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
What this PR does
In this PR I'm updating dskit to get grafana/dskit#217, which adds support for overriding the default list of accepted cipher suites and minimum TLS version for clients.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]